Skip to content

Conversation

@avi-alpert
Copy link
Contributor

Problem

Stuck on "Uploading code..." for /dev and /doc in a multi-root workspace with the same file paths in it multiple times.

Solution

Add a check to prevent same file from being added to archive twice

Fixes issue #6566


  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions
Copy link

  • This pull request modifies code in src/* but no tests were added/updated.
    • Confirm whether tests should be added or ensure the PR description explains why tests are not required.

const addedFilePaths = new Set()

for (const file of files) {
if (addedFilePaths.has(file.zipFilePath)) {
Copy link
Contributor

@vikshe vikshe Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does collectFiles returns same file path if sourcePaths/repoRootPaths > 1 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have a file that shows up in a multi root workspace more than once, then collectFiles will output it multiple times, all with the same zipFilePath

@avi-alpert avi-alpert marked this pull request as ready for review February 13, 2025 14:59
@avi-alpert avi-alpert requested review from a team as code owners February 13, 2025 14:59
@avi-alpert
Copy link
Contributor Author

Duplicate code CI failure: none of the files in this PR are listed

@avi-alpert
Copy link
Contributor Author

/runIntegrationTests

@jpinkney-aws
Copy link
Contributor

Can we create some tests for this?

@avi-alpert
Copy link
Contributor Author

Can we create some tests for this?

I confirmed that the unit test I added fails if I revert the fix, with the same error that causes the user facing bug.

const workspace2 = getWorkspaceFolder(folder.path + '/innerFolder')
const folderName = path.basename(folder.path)

await testPrepareRepoData([workspace1, workspace2], [`${folderName}_${workspace1.name}/${testFilePath}`])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're mostly just testing that this doesn't reject right? Does it make sense to wrap this with:

assert.doesNotReject(() => {
    await testPrepareRepoData([workspace1, workspace2], [`${folderName}_${workspace1.name}/${testFilePath}`])
})

? otherwise the PR looks good to me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the testPrepareRepoData has asserts inside of it, which will fail if the function fails

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error thrown by the test would be Error: File already exists at ZipWriter.add which indicates the root cause, the extra assert is not needed

@jpinkney-aws jpinkney-aws merged commit 5d26f9a into aws:master Feb 18, 2025
25 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants